Skip to content
This repository was archived by the owner on Aug 30, 2024. It is now read-only.

Add custom JSON encoder/decoder option to Document constructor#397

Merged
smithsz merged 1 commit intomasterfrom
allow-doc-specific-json-encoder-decoder
Jul 12, 2018
Merged

Add custom JSON encoder/decoder option to Document constructor#397
smithsz merged 1 commit intomasterfrom
allow-doc-specific-json-encoder-decoder

Conversation

@smithsz
Copy link
Copy Markdown
Contributor

@smithsz smithsz commented Jul 9, 2018

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

Allow a user to specify a custom JSON encoder and decoder for a Document type.

Approach

Pass additional kwargs encode & decode in via the Document constructor.

See test for example usage.

Schema & API Changes

Add additional kwargs to method call. No breaking API changes.

Security and Privacy

No change.

Testing

Adds tests/unit/client_tests.py:ClientTests.test_constructor_with_url.

Monitoring and Logging

No change.

Comment thread src/cloudant/document.py Outdated
if self._document_id is not None:
self['_id'] = self._document_id
self.encoder = self._client.encoder
self.decoder = kwargs.get('decoder') or self._client.decoder
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the decoder is not configurable, can we remove kwargs.get('decoder')?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only not configurable from the Cloudant client. We are allowing users to specify a custom decoder in the Document constructor.

A Client level decoder will be added in a future PR. It requires a little more work to ensure the decoder is passed around correctly.

Copy link
Copy Markdown
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just a couple of suggested improvements.

Comment thread tests/unit/document_tests.py Outdated
doc2.fetch()

self.assertEquals(doc2['name'], 'julia')
self.assertTrue(isinstance(doc2['dt'], datetime))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this and the following assertion be replaced by:

self.assertEquals(doc2['dt'], doc['dt'])

?

Since doc and doc2 should both be using the object form and the datetime equality should work here.

Comment thread src/cloudant/client.py Outdated
self.server_url = kwargs.get('url')
self._client_user_header = None
self.admin_party = admin_party
self.decoder = json.JSONDecoder # not currently configurable
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this already caused some confusion, perhaps append at client scope or similar.

Comment thread src/cloudant/document.py Outdated
if self._document_id is not None:
self['_id'] = self._document_id
self.encoder = self._client.encoder
self.decoder = kwargs.get('decoder') or self._client.decoder
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to document these parameters in the doc block.

@ricellis ricellis added this to the 2.next milestone Jul 10, 2018
@smithsz smithsz force-pushed the allow-doc-specific-json-encoder-decoder branch from 1a8b4b6 to 7e262e2 Compare July 11, 2018 08:29
Copy link
Copy Markdown
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@smithsz smithsz force-pushed the allow-doc-specific-json-encoder-decoder branch from 7e262e2 to 8201741 Compare July 11, 2018 14:14
@smithsz smithsz merged commit a2ec32f into master Jul 12, 2018
@smithsz smithsz deleted the allow-doc-specific-json-encoder-decoder branch July 12, 2018 09:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants